-
-
Notifications
You must be signed in to change notification settings - Fork 734
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: Replace several more uses of lodash with native, and related refactors #1754
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a fan of this in general, but I am slightly concerned about that changes that are not equivalent.
isString
covers strings created using the new
statement, typeof contentEncoding === 'string'
doesn't.
typeof value === 'object'
returns true for null
typeof value === 'function'
is false for async, generator, and proxy functions unlike isFunction
.
Since we don't plan on removing LoDash as dependency, my concern is that those particular changes will hamper deving in the future.
Yea, this is worth discussing. In a bit of searching, I haven't found any sources that argue for using String objects. Mostly people seem to discourage them, largely for this reason. There are probably plenty of places in nock where String objects work, though there clearly aren't tests around them, and for sure they don't work in all cases. For example, we use === to check equality, which doesn't work on String objects:
I guess the question is, do you think it's important to preserve some support for String objects? In the interest of caution, I could see an argument that dropping these is a breaking change, though that really depends whether people actually use these. We could also throw an error when given a String object, though then we'd still need an Re async functions and generators, I haven't been able to reproduce the issues you mention:
|
ah I didn't read the comment in LoDash's implementation of why/how then check for async/gens/etc. It has to do with a Safari quirk. As for the string stuff, I personally consider use the String constructor a smell, but given it's large user base it could bite us. |
We could do that instead. It seems a little nicer as
I suppose we could keep our own |
I'm on the fence, but would probably vote for using |
To be on the safe side we probably should change the string check in a breaking change. |
🎉 This PR is included in version 11.7.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
I'm facing a weird issue due to some changes in this PR. Specifically this. Here, the However, I do not have an explanation for this, so was wondering if any of you might have more insights on this? I tried debugging and put in a break point and verified this. The I'm using node 10 (even tried 13). I tried doing the same in the nock's tests but unfortunately I'm not able to reproduce it outside my codebase. I know it clearly implies that there must be something wrong with my setup, but no matter what the case is, why should an argument change from one step of the call stack to another! Would appreciate any ideas/suggestions to try things. |
Looks like this could be the reason. jestjs/jest#2549. And this seems like a good enough reason to go back to the |
I'm fine with returning to |
Cool, here's the PR to change it back to About tests, I'll try to put something together, but I'm not sure how much we'd cover if we just add the |
Either a single test of one instance would be fine, along with a comment explaining why this is necessary. If that's too hard, we could skip the test and instead put the comment next to one of these checks. |
Ok tried something like this, but it still works. i.e. gives const vm = require('vm')
function fn () {}
console.log(vm.runInNewContext('[typeof fn, fn instanceof Function]', { fn })) This is the case I was trying to reproduce, but not sure if that's easy enough. I've added the comment as you suggested, please take a look at the PR. |
This was resolved in #1850. |
🎉 This issue has been resolved in version 11.7.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This renames
Interceptor.matchAddress
toInterceptor.matchOrigin
which I think better fits what is being done in that method.The terminology comes from e.g. URL.origin and what is sent for an Origin header.
Ref #1285